OCPBUGS-78987: Add AWS environment variable flag to Cypress configuration and storage clone and snapshot tests cleanup#16181
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/valid-reference
|
@cajieh: This pull request references Jira Issue OCPBUGS-78987, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cajieh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
kind/cypress
|
/test e2e-gcp-console |
cajieh
force-pushed
the
add-aws-flag-cypress-config
branch
from
eb53375 to
603f159
Compare
cajieh
changed the title
cajieh
force-pushed
the
add-aws-flag-cypress-config
branch
from
603f159 to
57f2439
Compare
cajieh
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@cajieh: This pull request references Jira Issue OCPBUGS-78987, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
cajieh
force-pushed
the
add-aws-flag-cypress-config
branch
from
57f2439 to
824b8c9
Compare
|
@cajieh: This pull request references Jira Issue OCPBUGS-78987, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThis change refactors storage integration tests by introducing a new clone test mock module and centralizing naming constants across snapshot and clone fixtures. The clone.ts module exports PVC_NAME, CLONE_NAME, CLONE_SIZE, and a PVCGP3 variant for cross-storage-class testing. Snapshot.ts was restructured to export shared constants, replaced testerDeployment with testerDeploymentWithMounts, and moved PVCGP3 to the clone module. Both clone and snapshot test suites were rewritten to use direct cy.visit navigation instead of sidebar-based routing, adopt dvRows/dvFilter list page objects, and rely on UI-based assertions rather than oc command validation. The snapshot view selector was updated to use a data-test attribute, and BRIDGE_AWS environment variable support was added to the Cypress plugin configuration. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/packages/integration-tests-cypress/mocks/clone.ts (1)
1-5: Consider importingPVC_NAMEfrom snapshot.ts to avoid duplication.Both
clone.tsandsnapshot.tsexportPVC_NAME = 'testpvc'. Since you're already importingPVCfrom snapshot, importingPVC_NAMEas well would establish a single source of truth and prevent potential drift.♻️ Suggested refactor
-import { PVC } from './snapshot'; +import { PVC, PVC_NAME } from './snapshot'; -export const PVC_NAME = 'testpvc'; export const CLONE_NAME = `${PVC_NAME}-clone`; export const CLONE_SIZE = '2';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/integration-tests-cypress/mocks/clone.ts` around lines 1 - 5, Replace the duplicated PVC_NAME constant in clone.ts by importing PVC_NAME from snapshot.ts (you already import PVC), then use that imported PVC_NAME to build CLONE_NAME (`CLONE_NAME = `${PVC_NAME}-clone``) and keep CLONE_SIZE as-is; update the import line to include PVC_NAME and remove the local PVC_NAME declaration so clone.ts references the single source of truth from snapshot.ts.frontend/packages/integration-tests-cypress/tests/storage/clone.cy.ts (1)
93-119: Consider reducing duplication between clone creation tests.The "Creates PVC Clone" and "Creates PVC Clone with different storage class" tests share ~80% identical code. While E2E tests often favor explicitness over abstraction, this level of duplication could be reduced with a parameterized helper if test maintenance becomes burdensome. Current approach is acceptable—just flagging for awareness.
♻️ Example parameterized approach
const createCloneTest = (storageClass?: string) => { cy.exec(`oc delete pvc ${CLONE_NAME} -n ${testName} --ignore-not-found`, { failOnNonZeroExit: false, }); cy.visit(`/k8s/ns/${testName}/core~v1~PersistentVolumeClaim`); listPage.dvRows.shouldBeLoaded(); listPage.dvFilter.byName(PVC_NAME); listPage.dvRows.clickKebabAction(PVC_NAME, 'Clone PVC'); modal.shouldBeOpened(); modal.submitShouldBeEnabled(); cy.byTestID('input-request-size').clear().type(CLONE_SIZE); if (storageClass) { cy.byTestID('storage-class-dropdown').click(); cy.byTestID('console-select-item').contains(storageClass).click(); } modal.submit(); modal.shouldBeClosed(); // ... verification }; it('Creates PVC Clone', () => createCloneTest()); it('Creates PVC Clone with different storage class', () => createCloneTest('gp3-csi'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/integration-tests-cypress/tests/storage/clone.cy.ts` around lines 93 - 119, The two tests duplicate most steps (visiting PVC list, filtering by PVC_NAME, opening clone modal, setting size, optionally selecting storage class, submitting and verifying CLONE_NAME) so refactor by extracting a parameterized helper (e.g., createCloneTest(storageClass?: string)) that performs the shared flow using symbols from the diff: CLONE_NAME, testName, PVC_NAME, modal, listPage.dvRows, listPage.dvFilter, cy.byTestID('input-request-size'), cy.byTestID('storage-class-dropdown') and DetailsPageSelector for verification; then call it from the two tests (one with no arg and one with 'gp3-csi') and keep deletePVCClone(...) as-is.frontend/packages/integration-tests-cypress/tests/storage/snapshot.cy.ts (1)
31-34: Minor inconsistency:before()still uses sidebar navigation.The test setup uses
nav.sidenav.clickNavLinkwhile individual tests use directcy.visit(). Consider switching tocy.visit()here as well for consistency with the PR's navigation approach. Not blocking, as the current approach still works.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/integration-tests-cypress/tests/storage/snapshot.cy.ts` around lines 31 - 34, Replace the sidebar navigation call nav.sidenav.clickNavLink(['Storage', 'PersistentVolumeClaims']) in the test setup with a direct cy.visit(...) to the PersistentVolumeClaims list page (the same URL pattern used by individual tests) so the before() uses the same navigation method; ensure the visited URL lands on the PVC list such that listPage.dvRows.shouldBeLoaded() and listPage.dvFilter.byName(PVC_NAME) continue to work unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/packages/integration-tests-cypress/tests/storage/snapshot.cy.ts`:
- Around line 64-68: The test uses brittle `.first().click()` selection for PVC
and snapshot class (see cy.byTestID('pvc-dropdown') and
cy.byTestID('snapshot-dropdown') interacting with dropdownFirstItem), which can
pick the wrong item if ordering changes; instead locate and click the specific
option by its visible text or attribute (filter the dropdown items by the
expected name/value and then click that matched element) so the PVC and snapshot
class selections are based on content rather than position.
---
Nitpick comments:
In `@frontend/packages/integration-tests-cypress/mocks/clone.ts`:
- Around line 1-5: Replace the duplicated PVC_NAME constant in clone.ts by
importing PVC_NAME from snapshot.ts (you already import PVC), then use that
imported PVC_NAME to build CLONE_NAME (`CLONE_NAME = `${PVC_NAME}-clone``) and
keep CLONE_SIZE as-is; update the import line to include PVC_NAME and remove the
local PVC_NAME declaration so clone.ts references the single source of truth
from snapshot.ts.
In `@frontend/packages/integration-tests-cypress/tests/storage/clone.cy.ts`:
- Around line 93-119: The two tests duplicate most steps (visiting PVC list,
filtering by PVC_NAME, opening clone modal, setting size, optionally selecting
storage class, submitting and verifying CLONE_NAME) so refactor by extracting a
parameterized helper (e.g., createCloneTest(storageClass?: string)) that
performs the shared flow using symbols from the diff: CLONE_NAME, testName,
PVC_NAME, modal, listPage.dvRows, listPage.dvFilter,
cy.byTestID('input-request-size'), cy.byTestID('storage-class-dropdown') and
DetailsPageSelector for verification; then call it from the two tests (one with
no arg and one with 'gp3-csi') and keep deletePVCClone(...) as-is.
In `@frontend/packages/integration-tests-cypress/tests/storage/snapshot.cy.ts`:
- Around line 31-34: Replace the sidebar navigation call
nav.sidenav.clickNavLink(['Storage', 'PersistentVolumeClaims']) in the test
setup with a direct cy.visit(...) to the PersistentVolumeClaims list page (the
same URL pattern used by individual tests) so the before() uses the same
navigation method; ensure the visited URL lands on the PVC list such that
listPage.dvRows.shouldBeLoaded() and listPage.dvFilter.byName(PVC_NAME) continue
to work unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f6fc8ac7-e01d-4738-81f6-e457baa5fbe3
📒 Files selected for processing (6)
frontend/packages/integration-tests-cypress/mocks/clone.tsfrontend/packages/integration-tests-cypress/mocks/snapshot.tsfrontend/packages/integration-tests-cypress/plugins/index.jsfrontend/packages/integration-tests-cypress/tests/storage/clone.cy.tsfrontend/packages/integration-tests-cypress/tests/storage/snapshot.cy.tsfrontend/packages/integration-tests-cypress/views/storage/snapshot.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/integration-tests-cypress/plugins/index.jsfrontend/packages/integration-tests-cypress/views/storage/snapshot.tsfrontend/packages/integration-tests-cypress/mocks/clone.tsfrontend/packages/integration-tests-cypress/mocks/snapshot.tsfrontend/packages/integration-tests-cypress/tests/storage/snapshot.cy.tsfrontend/packages/integration-tests-cypress/tests/storage/clone.cy.ts
🧬 Code graph analysis (2)
frontend/packages/integration-tests-cypress/mocks/clone.ts (1)
frontend/packages/integration-tests-cypress/mocks/snapshot.ts (2)
PVC_NAME(3-3)PVC(89-104)
frontend/packages/integration-tests-cypress/tests/storage/clone.cy.ts (4)
frontend/packages/integration-tests-cypress/views/list-page.ts (1)
listPage(3-160)frontend/packages/integration-tests-cypress/support/index.ts (1)
testName(98-101)frontend/packages/integration-tests-cypress/mocks/snapshot.ts (3)
PVC(89-104)testerDeploymentWithMounts(64-87)PVC_NAME(3-3)frontend/packages/integration-tests-cypress/mocks/clone.ts (4)
PVCGP3(8-23)PVC_NAME(3-3)CLONE_NAME(4-4)CLONE_SIZE(5-5)
🔇 Additional comments (14)
frontend/packages/integration-tests-cypress/plugins/index.js (1)
69-69: LGTM!Clean addition following the established pattern for environment variable passthrough. This unblocks AWS-dependent tests without introducing any security or architectural concerns.
frontend/packages/integration-tests-cypress/views/storage/snapshot.ts (1)
8-8: LGTM!Excellent improvement switching to
data-testattribute selector. This decouples test stability from CSS class changes and aligns with console's testing conventions for resilient E2E selectors.frontend/packages/integration-tests-cypress/mocks/clone.ts (1)
7-23: LGTM!Clean composition reusing
PVCproperties while explicitly setting the different storage class. The JSDoc comment clarifies the test purpose well.frontend/packages/integration-tests-cypress/mocks/snapshot.ts (3)
3-5: LGTM!Good consolidation of naming constants. The derived
SNAPSHOT_NAMEand reference toDEPLOYMENT_NAMEestablish clear, maintainable naming conventions for the test fixtures.
63-87: LGTM with minor note.The
volumeMountsapproach is correct for filesystem-backed PVCs. The simplified spec (no nodeSelector/strategy overrides) is cleaner for test environments.One small observation: the JSDoc says "for clone tests" but
snapshot.cy.tsalso uses this deployment. Consider updating to "for storage tests requiring filesystem mounts" or similar.
116-124: LGTM!Using the template string ensures the restore PVC name stays synchronized with
SNAPSHOT_NAME. Good maintainability improvement.frontend/packages/integration-tests-cypress/tests/storage/snapshot.cy.ts (4)
1-20: LGTM!Clean imports with centralized constants. The AWS gating with
String(...).toLowerCase() === 'true'is defensive and handles edge cases well. Good inline documentation of platform requirements.
41-58: LGTM!Robust cleanup with
failOnNonZeroExit: falseensures test suite doesn't fail due to already-deleted resources. The--ignore-not-foundflag provides additional clarity of intent. Good defensive test teardown.
87-109: LGTM with design note.Good performance optimization moving the Ready status wait (120s) here from the create test. The approach of navigating to details first ensures the snapshot exists before attempting restore. The deployment patch to trigger Bound status is a solid verification approach.
Minor observation: the test navigates to details, waits for Ready, then navigates back to list to trigger restore via kebab. If the details page has a restore action, that could eliminate one navigation. Not blocking—current flow is clear and works.
81-85: LGTM!Clean list and delete tests using consistent
dvRowsAPI. Direct URL navigation and proper modal interaction/verification.Also applies to: 111-120
frontend/packages/integration-tests-cypress/tests/storage/clone.cy.ts (4)
10-20: LGTM!Good extraction of the delete workflow into a reusable helper. Clean parameterization and proper use of
dvRowsAPI with modal verification.
27-59: LGTM!Consistent setup/teardown pattern with
snapshot.cy.ts. Defensive cleanup withfailOnNonZeroExit: falseensures clean state regardless of test outcomes.
61-91: LGTM!Good test isolation with pre-cleanup of leftover clones. Direct navigation and
dvRowsAPI usage is consistent throughout. The 60s timeout for details page loading is appropriate for CI environments.
121-125: LGTM!Clear skip message indicating platform requirements when tests can't run.
| cy.byTestID('pvc-dropdown', { timeout: 60000 }).should('be.visible').click(); | ||
| cy.get(dropdownFirstItem, { timeout: 60000 }).should('be.visible').first().click(); | ||
| // Wait for snapshot class dropdown to be ready and select snapshot class | ||
| cy.byTestID('snapshot-dropdown', { timeout: 60000 }).should('be.visible').click(); | ||
| cy.get(dropdownFirstItem, { timeout: 60000 }).should('be.visible').first().click(); |
There was a problem hiding this comment.
Fragile dropdown selection: .first().click() assumes ordering.
Using .first() on dropdown items relies on a specific item ordering. If the dropdown contents change or load in different order, the test could select the wrong PVC or snapshot class. Consider filtering by expected name/value for more reliable selection.
🛡️ More resilient selection example
- cy.get(dropdownFirstItem, { timeout: 60000 }).should('be.visible').first().click();
+ cy.get(dropdownFirstItem, { timeout: 60000 }).contains(PVC_NAME).click();Similarly for snapshot class selection, if a specific class is expected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/packages/integration-tests-cypress/tests/storage/snapshot.cy.ts`
around lines 64 - 68, The test uses brittle `.first().click()` selection for PVC
and snapshot class (see cy.byTestID('pvc-dropdown') and
cy.byTestID('snapshot-dropdown') interacting with dropdownFirstItem), which can
pick the wrong item if ordering changes; instead locate and click the specific
option by its visible text or attribute (filter the dropdown items by the
expected name/value and then click that matched element) so the PVC and snapshot
class selections are based on content rather than position.
|
/test e2e-gcp-console |
| import { PVC } from './snapshot'; | ||
|
|
||
| export const PVC_NAME = 'testpvc'; |
There was a problem hiding this comment.
| import { PVC } from './snapshot'; | |
| export const PVC_NAME = 'testpvc'; | |
| import { PVC, PVC_NAME } from './snapshot'; |
There was a problem hiding this comment.
I intentionally scoped the const to this test to avoid cross-importing mocks, but a shared mock file would probably be cleaner.
| export const SNAPSHOT_NAME = `${PVC_NAME}-snapshot`; | ||
| export const DEPLOYMENT_NAME = 'busybox-deployment'; | ||
|
|
||
| export const testerDeployment = { |
There was a problem hiding this comment.
If testerDeployment is no longer used, it can be removed
| @@ -5,4 +5,4 @@ export namespace SnapshotDetails { | |||
There was a problem hiding this comment.
Lines 2-4 If no longer used, can be removed
| @@ -1,36 +1,40 @@ | |||
| import { PVC, PVCGP3, testerDeployment } from '../../mocks/snapshot'; | |||
| import { PVC_NAME, CLONE_NAME, CLONE_SIZE, PVCGP3 } from '../../mocks/clone'; | |||
There was a problem hiding this comment.
| import { PVC_NAME, CLONE_NAME, CLONE_SIZE, PVCGP3 } from '../../mocks/clone'; | |
| import { PVC_NAME, PVC, CLONE_NAME, CLONE_SIZE, PVCGP3 } from '../../mocks/clone'; |
|
/test e2e-gcp-console |
cajieh
force-pushed
the
add-aws-flag-cypress-config
branch
from
824b8c9 to
5bde779
Compare
cajieh
force-pushed
the
add-aws-flag-cypress-config
branch
from
5bde779 to
12a5820
Compare
3 participants
PR Summary
Problem
Missing AWS environment flag -
BRIDGE_AWSwasn't passed to Cypress, causing AWS-dependent tests to be skipped in AWS and other platforms.The snapshot.cy.ts and clone.cy.ts E2E tests were failing due to:
Incorrect URL paths - Snapshot creation form used wrong URL format (snapshot.storage.k8s.io
v1VolumeSnapshot instead of volumesnapshots)Wrong selectors - Used data-test="save-changes" instead of id="save-changes", and non-existent data-test="details-item-value__PVC"
Outdated list selectors - PVC lists now use DataView component (listPage.dvFilter/listPage.dvRows) instead of legacy selectors
Flaky sidebar navigation - Nav items hidden due to collapsed sections causing visibility: hidden errors
Fragile CLI assertions - cy.exec('oc get pvc...') failed with race conditions
Fixes
Cypress config: Added config.env.BRIDGE_AWS = process.env.BRIDGE_AWS to pass AWS flag (config change)
Navigation: Replaced sidebar clicks with direct cy.visit() URLs for reliability
Selectors: Updated to use correct element selectors and DataView helpers
Assertions: Removed non-existent data-test attributes and fragile CLI checks
Performance: Moved Ready status wait to restore test where it's actually needed
Justification
clone.cy.ts
snapshot.cy.ts
Summary by CodeRabbit